[ES|QL] RERANK command validation support
#221004
Open
+544
−78
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/ci |
vadimkibana
added
review
release_note:skip
|
Pinging @elastic/kibana-esql (Team:ESQL) |
botelastic
bot
added
the
ci:project-deploy-observability
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
@drewdaemon can you take this PR review? |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
|
drewdaemon
reviewed
May 27, 2025
Comment on lines
+92
to
+105
| export const isContainedLocation = (container: ESQLLocation, contained: ESQLLocation): boolean => { | ||
| return container.min <= contained.min && container.max >= contained.max; | ||
| }; | ||
|
|
||
| export const isContained = ( | ||
| container: { location?: ESQLLocation }, | ||
| contained: { location?: ESQLLocation } | ||
| ): boolean => { | ||
| if (!container.location || !contained.location) { | ||
| return false; | ||
| } | ||
|
|
||
| return isContainedLocation(container.location, contained.location); | ||
| }; |
There was a problem hiding this comment.
This may be redundant with within
Can we dedupe?
Comment on lines
+370
to
+375
| const isParentShowCommand = | ||
| !args.length && | ||
| (ctx.parent?.node as any)?.type === 'command' && | ||
| (ctx.parent?.node as any)?.name === 'show'; | ||
|
|
||
| const formatted = isParentShowCommand ? operator : `${operator}(${args})`; |
There was a problem hiding this comment.
So this is fixing an unrelated bug right?
There was a problem hiding this comment.
Can you explain the thinking behind this new pattern? (command definition living in its own module?)
Comment on lines
+418
to
+436
| /** | ||
| * Return nicely formatted human-readable out of parser errors. This callback | ||
| * lets commands construct their own error messages out of parser errors, | ||
| * as parser errors have the following drawbacks: | ||
| * | ||
| * 1. Not human-readable, even hard to read for developers. | ||
| * 2. Not translated to other languages, e.g. Chinese. | ||
| * 3. Depend on ANTLR grammar, which is not stable and may change in the future. | ||
| * | ||
| * @param parsingErrors List of parsing errors returned by the ANTLR parser | ||
| * for this command. | ||
| * @returns Human-readable, translatable messages for the user. | ||
| */ | ||
| parsingErrorsToMessages?: ( | ||
| parsingErrors: EditorError[], | ||
| command: ESQLCommand<CommandName>, | ||
| references: ReferenceMaps | ||
| ) => ESQLMessage[]; | ||
|
|
There was a problem hiding this comment.
Till now we have relied on a clean passthrough of ANTLR errors despite their limitations. This new idea raises the following questions for me
- When is this expected?
- Do we need to add these for every new command?
- Which messages are important enough to give this special treatment to? Surely not all messages?
- We don't control the parser messages. How often will they change in our grammar update PRs, requiring us to update this logic?
- Is it ok to introduce this on an ad-hoc basis (as we have here) or should we do it holistically for all commands (if we're going to do it)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Partially addresses #217285
Checklist